Repository navigation
fix(compare): stop the midnight tariff comparison changing the live plan's next decision - #5465
Open
chalfontchubby wants to merge 4 commits into
Open
chalfontchubby wants to merge 4 commits into
chalfontchubby wants to merge 4 commits into
Conversation
…iff comparison Plan selection scores the pre-clip version of each plan (#4403), keeping the plan in force's copy in plan_preclip for the next re-plan to score as the previous plan. The tariff comparison runs calculate_plan() for every tariff, each replacing plan_preclip with that tariff's 48-hour plan, and restored the windows and limits but not plan_preclip. The first re-plan after the midnight comparison therefore scored the last tariff's plan as the plan in force: on 9 Oct 2026 (Sigenergy, IOG) live scored its previous plan at 300.97p where the same plan scores 30.71p without the comparison, and so switched plans regardless of metric_min_improvement_plan. Found with the forward log replay (#5363): running the comparison inside the replay reproduced the inflated score, and restoring each attribute the comparison changes, group by group, isolated plan_preclip. run_all() now saves and restores it with the windows; a test runs run_all() with a tariff that replaces it and checks it comes back (it fails without the fix). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… each tariff from scratch The comparison kept a hand-written list of attributes to save and restore, and plan_preclip leaked because it was missing from it; end_record, the two rate_best_cost thresholds and plan_valid / plan_last_updated were missing too, so after the comparison the next cycle published the last tariff's thresholds and treated the restored plan as freshly computed. run_all() now saves and restores the attributes declared in COMPARE_PLAN_STATE, COMPARE_HARDWARE_STATE (also reset after each tariff) and the rest of COMPARE_RESTORED_STATE, with the car state in COMPARE_RESTORED_COPIES deep-copied as before. Each tariff now plans from scratch (plan_valid False before its run): scored against the previous tariff's plan, the keep-or-switch threshold in calculate_plan() could leave one tariff's result as another tariff's plan. Tests: run_all() with a tariff run that changes every saved attribute must restore all of them, reset the hardware values between tariffs and start each tariff with plan_valid False; and every attribute calculate_plan() assigns must be either in COMPARE_PLAN_STATE or listed as rebuilt every run, so the next new piece of plan state fails a test rather than leaking (checked: dropping end_record from the set fails it). T19 and T25 now check the declared sets rather than the old source text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tore state even if the config restore raises Two gaps a cold review found in the restore: - Restoring plan_valid put back the True from before the comparison, overwriting a re-plan requested while it ran (an HA event, the web UI or an agent tool changing a setting sets update_pending and clears plan_valid). The next run would then treat the pre-change plan as valid. plan_valid is now restored as it was only if no re-plan is pending. - fetch_config_options() runs first in the final restore; if it raised, none of the saved state was put back - the leak this branch fixes. The attribute restore now runs in a finally around the config restore, still last so it wins. The coverage check now reads calculate_plan() and every optimise pass with ast, so augmented, annotated and tuple assignments to self are found too (nothing new is uncovered today). New tests: a re-plan requested during the comparison leaves the plan invalid, and the plan is restored when the config restore raises; both fail without this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
run_all() now ends with plan_valid = saved and not update_pending. The shared fixture starts with update_pending True and several earlier tests leave plan_valid True, so test_run_all_restores_state failed or passed depending on run order. Set a valid plan with no re-plan pending before it, and save and restore both flags in it and in test_run_all_restores_when_config_restore_raises, which left plan_valid False for every later test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem, in one line
For anyone with a
compare_list, the tariff comparison that runs at midnight leaves part of the comparison's plan behind on the live instance, so the first re-plan after midnight decides whether to keep the current plan by scoring the wrong plan - and always switches.What happens today
Plan selection scores the pre-clip version of each plan (#4403) and keeps the plan in force's copy in
plan_preclip. At the next re-plan that copy is "the previous plan" the new one must beat bymetric_min_improvement_plan(default 2p) to replace it.The comparison runs
calculate_plan()once per tariff on the live instance. Each run replacesplan_preclipwith that tariff's 48-hour plan.run_all()put the windows and limits back afterwards, but notplan_preclip- so at the first re-plan after midnight, "the previous plan" was really the last compared tariff's plan.Seen live on 9 Oct 2026 (Sigenergy, Intelligent Octopus Go), 00:10:
Scored correctly, the plan in force at that moment is worth 30.71p - the new plan was no better, and the plan should have been kept. Instead every night the first post-midnight re-plan switches plans regardless of the threshold, while a charge window may be running. On this night it ended the overnight charge about half an hour early, and the battery started the day at 62% rather than 72% in a simulation of the same night.
The same review found the comparison also leaves, until the next re-plan: the last tariff's charge/export cost thresholds (published to HA and used to colour the plan), its
end_record, and aplan_last_updatedthat makes the restored plan look freshly computed (delaying the next scheduled re-plan).How it was found and pinned down
With the forward log replay (#5363 / #5427), replaying the live log through midnight:
plan_preclip.The fix (compare.py only)
run_all()now saves and restores a declared set of state -COMPARE_PLAN_STATE(windows, limits,plan_preclip,end_record,plan_valid,plan_last_updated, the two cost thresholds), the hardware overrides (still reset after each tariff too), and the existing manual-time / today-counter / car state - instead of a hand-written list, which is howplan_preclipwas missed.plan_valid = Falsefirst), so a tariff's result is no longer held to the previous tariff's plan by the keep-or-switch threshold.finallyaround the config restore, so it happens even iffetch_config_options()raises; and a re-plan requested during the comparison (a setting changed at 00:01) is kept rather than overwritten.No change to how the live plan is made. The only behaviour change outside the restore is that each compared tariff is now planned on its own merits, so the comparison's per-tariff results may shift slightly - arguably what they should always have been.
Tests (each fails on main)
test_run_all_restores_state: a tariff run that changes all 41 saved attributes; every one is restored, hardware is reset between tariffs, and each tariff starts withplan_validFalse.test_plan_state_covered: everyself.<name>assigned incalculate_plan()and the optimise passes (read withast) must be inCOMPARE_PLAN_STATEor listed as rebuilt every run - so the next new piece of plan state fails a test instead of leaking.test_run_all_keeps_pending_replan,test_run_all_restores_when_config_restore_raises.Full suite and pre-commit pass on each of the three commits.
How to check it yourself
On a system with a
compare_list, grep the log for the firstPrevious plan best metricafterStarting comparison of tariffs: on main the previous-plan figure is wildly out of line with the plan in force (several times the new plan's); with this change it is in line, and the plan is kept unless the new one is genuinely better.🤖 Generated with Claude Code